Prepared statements#23156
Draft
trasher wants to merge 1 commit into
Draft
Conversation
trasher
force-pushed
the
feature/prepared-statements
branch
4 times, most recently
from
b9fbbae to
22f869e
Compare
trasher
force-pushed
the
feature/prepared-statements
branch
2 times, most recently
from
688c58b to
2b65826
Compare
trasher
force-pushed
the
feature/prepared-statements
branch
9 times, most recently
from
95c0aed to
9b9c0cd
Compare
trasher
commented
Mar 12, 2026
trasher
commented
Mar 12, 2026
cedric-anne
reviewed
Mar 12, 2026
trasher
force-pushed
the
feature/prepared-statements
branch
2 times, most recently
from
f833f2f to
6c98d07
Compare
trasher
force-pushed
the
feature/prepared-statements
branch
2 times, most recently
from
374aae7 to
9117f26
Compare
trasher
force-pushed
the
feature/prepared-statements
branch
from
9117f26 to
5f730a1
Compare
cedric-anne
reviewed
Apr 29, 2026
Member
cedric-anne
left a comment
cedric-anne
left a comment
There was a problem hiding this comment.
The proposed way to migrate to prepared queries is globally OK for me.
| * @param string $expression The query expression | ||
| * @param ?string $alias The query expression alias | ||
| */ | ||
| public function __construct($expression, ?string $alias = null) |
Member
There was a problem hiding this comment.
It may help to produce less verbose code, for instance new QueryExpression('IF(field, ?, ?)', values: ['foo', 'bar'], alias: 'whatever').
Suggested change
| public function __construct($expression, ?string $alias = null, array $values = []) |
| }; | ||
| $func_name = strtoupper($func_name); | ||
| return self::getExpression($func_name, $params, $args[1] ?? null); | ||
| //FIXME: no idea how to handle possible statement values, and even if there are some... |
Member
There was a problem hiding this comment.
I think we should not try to handle them. I even think we should deprecate this magic method, and declare simple static methods that corresponds to used functions. Anyway, it is out of the scope of this PR.
| ] | ||
| ); | ||
| } catch (StatementException $e) { | ||
| if (!str_contains($e->getMessage(), 'Duplicate entry')) { |
Member
There was a problem hiding this comment.
A Duplicate entry is unexpected here, since dusplicates are supposed to be cleaned on lines 1100-1102.
|
|
||
| $this->expectExceptionMessage( | ||
| '`Computer::getFromDBByRequest()` expects to get one result, 2 found in query "SELECT `glpi_computers`.* FROM `glpi_computers` WHERE `contact` = \'johndoe\'".' | ||
| '`Computer::getFromDBByRequest()` expects to get one result, 2 found in query "SELECT `glpi_computers`.* FROM `glpi_computers` WHERE `contact` = ?".' |
Member
There was a problem hiding this comment.
Is there a way to preserve values in the exception message ?
|
|
||
| namespace Glpi\DBAL; | ||
|
|
||
| abstract class Prepared |
Member
There was a problem hiding this comment.
Maybe the names of the new classes could be more precise. For instance AbstractPreparedQuery, DeletePreparedQuery, ...
Rework affected rows (must be retrieved from statement) Handle Prepared from migrations Handle possible exisiting QueryExpression statement values Try to detect early bind issues Limit to simple cases for now Fix prepared IN, fix buildUpdate Try to get extra information on failure rawAnalyzeCrit (maybe to remove)
trasher
force-pushed
the
feature/prepared-statements
branch
from
87349ba to
f84f3c7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
WIP.
Currently, only "CRUD" related methods have been reworked; as well as DBIterator, and Query* classes.
Since Search engine relies on some f them but also have its own code; it's currently in a non working state. It's the next step; I began to work on this already.
HLAPI tests are also broken; I have no idea how to fix, nor if it's complex or not.
There also is a (minor) problem with 0.85 migration data. seems like the query and parameters are correct regarding sent criteria; and I've not been able to reproduce on my local instance (while I've spotted several other issues on that migration :/)